Skip to content

Fix C++ compilation modes and preserve C linkage - #930

Draft
anemitz wants to merge 2 commits into
RoaringBitmap:masterfrom
anemitz:fix/c-tests-cpp
Draft

Fix C++ compilation modes and preserve C linkage#930
anemitz wants to merge 2 commits into
RoaringBitmap:masterfrom
anemitz:fix/c-tests-cpp

Conversation

@anemitz

@anemitz anemitz commented Oct 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR restores ROARING_BUILD_C_TESTS_AS_CPP and ROARING_BUILD_C_AS_CPP, including the external C function definitions needed when C callers link against a C++-built library.

Why care?

For it:

  • The options already exist and are broken. They date from 2020 (Add C++ Namespaces (retry to trigger Appveyor) #253). Their stated purpose was that compiling C as C++ "is a useful test for catching certain errors" and enables C++ static analysis. They have never been in CI and have quietly broken: on master, g++ can't compile bitset.c as C++. A build option that doesn't build is worse than no option.
  • The linkage part is needed for the option to be usable. A C++-built library that C programs can't reliably link against defeats the purpose of building the library.
  • CI keeps it from breaking again. Without CI, these modes break silently.

Against it:

  • Nobody seems to use the modes. I found no upstream issues about them, and they've been broken unnoticed.
  • It found no real C bugs. The fixes it forced (the casts, the MM_PERM* constants) are C++ compatibility changes, not bug fixes in C.
  • It adds ongoing cost.
    • Every new header inline function must use the macro.
    • Every push runs 8 more CI jobs.
    • There's a new shell script to maintain.
    • C++ builds export 45 extra symbols.
  • MSVC still doesn't work. That's the toolchain where someone would most likely compile C as C++.
Validation and implementation notes
  • GCC 15 and Clang 22 on Linux ARM64: all four static C/C++ library/test combinations and the shared C++ library with C tests pass, 28 tests per configuration (280 total).
  • All 562 global C function symbols are present in the C++ static and shared libraries under both compilers. The C++ amalgamation also preserves all 562 symbols.
  • GCC 13 on x86-64 Ubuntu 24.04, with the workflow's commands: the C++ library with C tests (static and shared) and the C++ library with C++ tests pass, 28 tests each, and all 578 C function symbols are present in the C++ static and shared libraries according to GNU nm. The ARM64 runs above compile out the AVX-512 code, which is how the _MM_PERM_ENUM error first reached CI.
  • The linkage regression test and symbol comparison both fail against a library lacking container_iterator_next and container_iterator_prev.
  • AppleClang: Debug shared build and seven focused tests pass; the Release x86_64 C linkage test passes under Rosetta, and the Release static C linkage test passes with LTO.
  • Clang-format 17, YAML parsing, shell syntax, and symbol-checker error-path checks pass.

The C++ build emits 45 constant croaring_inline_* pointer symbols as implementation details to retain callable inline definitions without compiler-specific retention flags. Static-inline helpers are not retained this way.

anemitz and others added 2 commits October 30, 2026 20:43
@anemitz
Fix C++ compilation modes and preserve C linkage
275fd69
@anemitz @claude
Build the AVX-512 merge as C++ with GCC and drop the MSVC jobs
f9f8187 
GCC declares the immediate of _mm512_shuffle_epi32 as _MM_PERM_ENUM, and
C++ does not convert an int to it, so the library failed to compile as
C++ with GCC on x86-64. Pass _MM_PERM_BADC and _MM_PERM_CDAB, the named
constants for 0x4E and 0xB1.

The library does not compile as C++ under MSVC, which rejects C
designated initializers before C++20 and C compound literals in any
mode. Remove the Windows jobs until that is fixed, and rename the
workflow to match the other Ubuntu-only workflows.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@anemitz

anemitz commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Perhaps the best course of action is to just remove these broken options since it's unclear if there's value in fixing this.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@anemitz